[TooltipWithBounds] fix : overflowing its parent on small screens #837

LethalPants · 2020-10-02T07:14:37Z

🐛 Bug Fix

TooltipWithBounds overflowing its parent on small screens #466 : change logic to calculate position

coveralls · 2020-10-02T07:23:20Z

Pull Request Test Coverage Report for Build 297437244

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 14 (0.0%) changed or added relevant lines in 1 file are covered.
41 unchanged lines in 8 files lost coverage.
Overall coverage increased (+1.8%) to 57.086%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/visx-tooltip/src/tooltips/TooltipWithBounds.tsx	0	14	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/visx-pattern/src/patterns/Lines.tsx	1	85.0%
packages/visx-xychart/src/components/XYChart.tsx	2	92.5%
packages/visx-tooltip/src/hooks/useTooltip.ts	3	44.44%
packages/visx-responsive/src/enhancers/withParentSize.tsx	5	58.06%
packages/visx-xychart/src/components/series/BarSeries.tsx	5	58.44%
packages/visx-xychart/src/components/series/LineSeries.tsx	5	40.63%
packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx	7	0.0%
packages/visx-responsive/src/components/ParentSize.tsx	13	0.0%

Totals
Change from base Build 283122090:	1.8%
Covered Lines:	1285
Relevant Lines:	2184

💛 - Coveralls

williaster · 2020-10-02T21:41:40Z

Hey @LethalPants thanks for the PR! In playing with this locally there are a couple of issues:

leftOffset/topOffset are not applied correctly when the tooltip is positioned to the bottom or the right (they are flush with the lines but there should be 10px of offset padding)

to tooltipInPortal is always positioned to the bottom left regardless of its position

LethalPants · 2020-10-03T05:00:22Z

Okay I'll take a look at it and make the changes. Thanks!

joshxyzhimself · 2020-10-03T10:27:19Z

Hi @LethalPants this looks gangsta, just curious do you think this is also related to #839?

LethalPants · 2020-10-03T21:09:43Z

to tooltipInPortal is always positioned to the bottom left regardless of its position

I don't think this and the overflow issue my commit fixes are related as it is persistent in the current version too, just that the tooltip is in the bottom right not sure if that was planned.

Unrelated but the tooltip is under the plot in the published docs. 👀

… offset in all quadrants

LethalPants · 2020-10-04T11:09:47Z

I've changed the logic so that the position of the tooltip is change when the gap is less than 1% of the width of parentBounds
Which eliminated the offset issue mentioned by @williaster. As for the issue it's not entirely possible to fix it with a fixed width of the tooltip even if it's placed in the largest quadrant. We should probably make the tooltip responsive.

williaster

@LethalPants sorry for the delay, thanks for iterating on this! After playing with this version, I think something like the following might be the ideal fix:

// parentBounds.width = 0 when rendered in a `Portal`
const placeTooltipLeft = parentBounds.width
  ? left + offsetLeft > parentBounds.width / 2
  : left + offsetLeft > window.innerWidth / 2;

const placeTooltipUp = parentBounds.height
  ? top + offsetTop > parentBounds.height / 2
  : top + offsetTop > window.innerHeight / 2;

I think this has two benefits versus the 1% implementation:

This always puts the tooltip in the quadrant with the most space (even if it's clipped; to avoid this I agree the Tooltip needs to have a dynamic width). With the 1% logic, if the tooltip will overflow on either side, it will still choose the side with less space (which is the bug report)

1% logic (current)

largest quadrant logic (proposed)

This logic doesn't depend on ownBounds.left/right, which actually is a great performance win. If you check the example code, we often use key={Math.random()} for the tooltip to force update ownBounds, but that re-mounts the component on every mouse move. If we don't need ownBounds, we can remove that and improve perf pretty significantly.

williaster · 2020-10-06T19:55:11Z

Thanks for the 🐛 report for the z-index issue in the airbnb.io/visx site, I noticed this as well and created an issue #850

williaster · 2020-10-06T20:57:35Z

Sorry, playing with my suggested code more also isn't 100% perfect for the Portal case because parentBounds is basically the entire page. So if you do > parentBounds.[width or height] / 2 it has weird effects depending where on the page the tooltip is used 🤦 .

I think this is better, but it isn't as robust to not needing the key={Math.random} (ownBounds.width/height is more stable than ownBounds.left/top/right/bottom would be, and we still avoid referencing the latter):

const placeTooltipLeft = parentBounds.width
  ? left + offsetLeft > parentBounds.width / 2
  : left + offsetLeft + ownBounds.width > window.innerWidth;
const placeTooltipUp = parentBounds.height
  ? top + offsetTop + ownBounds.height > parentBounds.height
  : top + offsetTop + ownBounds.height > window.innerHeight;

LethalPants · 2020-10-07T05:02:50Z

My initial solution used left > parentBounds.width / 2 which seemed to work almost fine but seeing the tooltip shift position at the halfway mark didn't feel natural 😅

So if you do > parentBounds.[width or height] / 2 it has weird effects depending where on the page the tooltip is used 🤦.

Can you please elaborate on this, I couldn't recreate the issue you mentioned.

I'm not sure whether the tooltip is supposed to stay in the bounds of the plot but that isn't the case in the code snippet you pasted.

williaster · 2020-10-07T21:10:39Z

@LethalPants yeah I agree that the shift at the midway point doesn't feel as great UX wise. okay what about this, it doesn't rely on left/right/top/bottom, swaps position only when it encounters the right/bottom edge, and swaps to the side where the fewest px will be clipped (the original bug). for very wide tooltips that don't fit on either side this will swap the position at the parent's mid-width point (in this case the user should render the Tooltip in a Portal, or make its width responsive)

let placeTooltipLeft = false;

if (parentBounds.width) {
  const rightPlacementClippedPx = left + offsetLeft + ownBounds.width - parentBounds.width;
  const leftPlacementClippedPx = ownBounds.width - left - offsetLeft;
  placeTooltipLeft =
    rightPlacementClippedPx > 0 && rightPlacementClippedPx > leftPlacementClippedPx;
} else {
  placeTooltipLeft = left + offsetLeft + ownBounds.width > window.innerWidth;
}

const placeTooltipUp = parentBounds.height
  ? top + offsetTop + ownBounds.height > parentBounds.height
  : top + offsetTop + ownBounds.height > window.innerHeight;

I'm not sure whether the tooltip is supposed to stay in the bounds of the plot but that isn't the case in the code snippet you pasted.

When Portal is enabled, the tooltip is rendered outside of the container, and so its true parent container is <body />. So in this case it's not supposed to detect its immediate container's bounds, instead it should detect the page bounds (the above solution should work for this).

So if you do > parentBounds.[width or height] / 2 it has weird effects depending where on the page the tooltip is used 🤦.

Can you please elaborate on this, I couldn't recreate the issue you mentioned.

This is confusing, sorry. It only applies to the Portal case where parentRect is the <body />, in some of the solutions we discussed where we flipped position based on parent width/height, there was weird behavior on /gallery and /docs/tooltip where the tooltip example is further down the page. I think we don't have to worry about this if we go with the above solution. sorry this turned out to be more complicated than initially anticipated 🙉 .

LethalPants · 2020-10-09T08:07:47Z

Sorry for the delay.

what about this, it doesn't rely on left/right/top/bottom, swaps position only when it encounters the right/bottom edge, and swaps to the side where the fewest px will be clipped (the original bug).

This seems fine.

for very wide tooltips that don't fit on either side this will swap the position at the parent's mid-width point (in this case the user should render the Tooltip in a Portal, or make its width responsive)

I think changing to Portal would be a better option, incase the tooltip is wider than parentBounds.width * 0.75 we can switch it to Portal, sounds good?

I've also pushed changes for detecting the edge with the snippet you posted.

williaster

@LethalPants this looks great 👌 going to go ahead and merge, thanks for all the iteration on this!

I think changing to Portal would be a better option, incase the tooltip is wider than parentBounds.width * 0.75 we can switch it to Portal, sounds good?

I think auto-switching is pretty tricky at the moment because rendering in the Portal currently requires a ResizeObserver polyfill so it'd be a breaking change. I think in most cases a Portal is actually more ideal, and useTooltipInPortal is compatible with TooltipWithBounds so users can leverage these enhancements.

LethalPants added 2 commits October 2, 2020 10:01

[TooltipWithBounds] change logic to calculate position

355632d

format : TooltipWithBounds

a719409

[TooltipWithBounds] update: change boundary on width of parent; apply…

96b7c90

… offset in all quadrants

hshoff added the @visx/tooltip label Oct 5, 2020

williaster reviewed Oct 6, 2020

View reviewed changes

williaster added the 🐛 bug label Oct 7, 2020

fix: [tooltip] move to quadrant where least clipping happens

44e6b46

williaster approved these changes Oct 12, 2020

View reviewed changes

williaster merged commit aec0d09 into airbnb:master Oct 12, 2020

williaster added this to the 1.1.0 milestone Oct 17, 2020

m0t0r mentioned this pull request Oct 31, 2020

TooltipWithBounds overflowing its parent on small screens #466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TooltipWithBounds] fix : overflowing its parent on small screens #837

[TooltipWithBounds] fix : overflowing its parent on small screens #837

LethalPants commented Oct 2, 2020

coveralls commented Oct 2, 2020 •

edited

Loading

williaster commented Oct 2, 2020

LethalPants commented Oct 3, 2020

joshxyzhimself commented Oct 3, 2020

LethalPants commented Oct 3, 2020 •

edited

Loading

LethalPants commented Oct 4, 2020 •

edited

Loading

williaster left a comment •

edited

Loading

williaster commented Oct 6, 2020

williaster commented Oct 6, 2020 •

edited

Loading

LethalPants commented Oct 7, 2020 •

edited

Loading

williaster commented Oct 7, 2020 •

edited

Loading

LethalPants commented Oct 9, 2020 •

edited

Loading

williaster left a comment

[TooltipWithBounds] fix : overflowing its parent on small screens #837

[TooltipWithBounds] fix : overflowing its parent on small screens #837

Conversation

LethalPants commented Oct 2, 2020

🐛 Bug Fix

coveralls commented Oct 2, 2020 • edited Loading

Pull Request Test Coverage Report for Build 297437244

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

williaster commented Oct 2, 2020

LethalPants commented Oct 3, 2020

joshxyzhimself commented Oct 3, 2020

LethalPants commented Oct 3, 2020 • edited Loading

LethalPants commented Oct 4, 2020 • edited Loading

williaster left a comment • edited Loading

Choose a reason for hiding this comment

williaster commented Oct 6, 2020

williaster commented Oct 6, 2020 • edited Loading

LethalPants commented Oct 7, 2020 • edited Loading

williaster commented Oct 7, 2020 • edited Loading

LethalPants commented Oct 9, 2020 • edited Loading

williaster left a comment

Choose a reason for hiding this comment

coveralls commented Oct 2, 2020 •

edited

Loading

LethalPants commented Oct 3, 2020 •

edited

Loading

LethalPants commented Oct 4, 2020 •

edited

Loading

williaster left a comment •

edited

Loading

williaster commented Oct 6, 2020 •

edited

Loading

LethalPants commented Oct 7, 2020 •

edited

Loading

williaster commented Oct 7, 2020 •

edited

Loading

LethalPants commented Oct 9, 2020 •

edited

Loading